EIP-7549: validator client #14158

rkapka · 2024-06-28T16:38:56Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Implements the following changes for EIP-7549

validator client code
- submitting Electra attestations
- submitting Electra aggregates
server code
- ProposeAttestationElectra

prestonvanloon

Some suggestions, overall looks good though

prestonvanloon · 2024-07-02T17:23:52Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_attestations.go

+	versionAtts := make([]ethpb.Att, 0, len(atts))
+	if postElectra {
+		for _, a := range atts {
+			if a.Version() == version.Electra {
+				versionAtts = append(versionAtts, a)
+			}
+		}
+	} else {
+		for _, a := range atts {
+			if a.Version() == version.Phase0 {
+				versionAtts = append(versionAtts, a)
+			}
+		}
+	}


Can you add tests for this function logic? If it's too complicated, consider moving this filtering logic to its own method and testing that.

prestonvanloon · 2024-07-02T17:24:29Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_attestations.go

+	if len(a) == 0 {
+		return a
+	}
+
+	var limit uint64
+	if a[0].Version() == version.Phase0 {
+		limit = params.BeaconConfig().MaxAttestations
+	} else {
+		limit = params.BeaconConfig().MaxAttestationsElectra
+	}
+	if uint64(len(a)) > limit {
+		return a[:limit]


Should this be tested?

prestonvanloon · 2024-07-02T17:25:12Z

beacon-chain/rpc/prysm/v1alpha1/validator/attester.go

-		},
-	})
+	go func() {
+		ctx = trace.NewContext(context.Background(), trace.FromContext(ctx))


What is the purpose of this line?

Suggested change

ctx = trace.NewContext(context.Background(), trace.FromContext(ctx))

I don't know, but it was there previously. I am a little scared to change this

It's safe to delete it.

What this line of code does is "create a new context, which does not inherit any deadline from the parent, but includes all of the trace/span metadata from the parent context".

However, this isn't even used... so let's delete it

beacon-chain/rpc/prysm/v1alpha1/validator/attester.go

prestonvanloon · 2024-07-02T17:43:18Z

validator/client/attest.go

-		Data:            data,
-		AggregationBits: aggregationBitfield,
-		Signature:       sig,
+	// TODO: Extend to Electra


What is this TODO?

This is slasher logic which hasn't been updated to Electra yet. It is non-trivial and needs more discussion.

prestonvanloon · 2024-07-02T17:47:25Z

validator/client/attest.go

+	if postElectra {
+		span.AddAttributes(
+			trace.Int64Attribute("slot", int64(slot)), // lint:ignore uintcast -- This conversion is OK for tracing.
+			trace.StringAttribute("attestationHash", fmt.Sprintf("%#x", attResp.AttestationDataRoot)),
+			trace.StringAttribute("committeeBitfield", fmt.Sprintf("%#x", committeeBits)),
+			trace.StringAttribute("blockRoot", fmt.Sprintf("%#x", data.BeaconBlockRoot)),
+			trace.Int64Attribute("justifiedEpoch", int64(data.Source.Epoch)),
+			trace.Int64Attribute("targetEpoch", int64(data.Target.Epoch)),
+			trace.StringAttribute("aggregationBitfield", fmt.Sprintf("%#x", aggregationBitfield)),
+		)
+	} else {
+		span.AddAttributes(
+			trace.Int64Attribute("slot", int64(slot)), // lint:ignore uintcast -- This conversion is OK for tracing.
+			trace.StringAttribute("attestationHash", fmt.Sprintf("%#x", attResp.AttestationDataRoot)),
+			trace.Int64Attribute("committeeIndex", int64(data.CommitteeIndex)),
+			trace.StringAttribute("blockRoot", fmt.Sprintf("%#x", data.BeaconBlockRoot)),
+			trace.Int64Attribute("justifiedEpoch", int64(data.Source.Epoch)),
+			trace.Int64Attribute("targetEpoch", int64(data.Target.Epoch)),
+			trace.StringAttribute("aggregationBitfield", fmt.Sprintf("%#x", aggregationBitfield)),
+		)
+	}


Suggested change

if postElectra {

span.AddAttributes(

trace.Int64Attribute("slot", int64(slot)), // lint:ignore uintcast -- This conversion is OK for tracing.

trace.StringAttribute("attestationHash", fmt.Sprintf("%#x", attResp.AttestationDataRoot)),

trace.StringAttribute("committeeBitfield", fmt.Sprintf("%#x", committeeBits)),

trace.StringAttribute("blockRoot", fmt.Sprintf("%#x", data.BeaconBlockRoot)),

trace.Int64Attribute("justifiedEpoch", int64(data.Source.Epoch)),

trace.Int64Attribute("targetEpoch", int64(data.Target.Epoch)),

trace.StringAttribute("aggregationBitfield", fmt.Sprintf("%#x", aggregationBitfield)),

)

} else {

span.AddAttributes(

trace.Int64Attribute("slot", int64(slot)), // lint:ignore uintcast -- This conversion is OK for tracing.

trace.StringAttribute("attestationHash", fmt.Sprintf("%#x", attResp.AttestationDataRoot)),

trace.Int64Attribute("committeeIndex", int64(data.CommitteeIndex)),

trace.StringAttribute("blockRoot", fmt.Sprintf("%#x", data.BeaconBlockRoot)),

trace.Int64Attribute("justifiedEpoch", int64(data.Source.Epoch)),

trace.Int64Attribute("targetEpoch", int64(data.Target.Epoch)),

trace.StringAttribute("aggregationBitfield", fmt.Sprintf("%#x", aggregationBitfield)),

)

}

span.AddAttributes(

trace.Int64Attribute("slot", int64(slot)), // lint:ignore uintcast -- This conversion is OK for tracing.

trace.StringAttribute("attestationHash", fmt.Sprintf("%#x", attResp.AttestationDataRoot)),

trace.StringAttribute("committeeBitfield", fmt.Sprintf("%#x", committeeBits)),

trace.StringAttribute("blockRoot", fmt.Sprintf("%#x", data.BeaconBlockRoot)),

trace.Int64Attribute("justifiedEpoch", int64(data.Source.Epoch)),

trace.Int64Attribute("targetEpoch", int64(data.Target.Epoch)),

trace.StringAttribute("aggregationBitfield", fmt.Sprintf("%#x", aggregationBitfield)),

)

if postElectra {

span.AddAttributes(

trace.StringAttribute("committeeBitfield", fmt.Sprintf("%#x", committeeBits)),

)

} else {

span.AddAttributes(

trace.Int64Attribute("committeeIndex", int64(data.CommitteeIndex)),

)

}

This reverts commit f59e1459f3f7561e0483bc8542110794951585c5.

saolyn · 2024-07-11T09:33:59Z

beacon-chain/rpc/prysm/v1alpha1/validator/attester.go

+
+	root, err := att.GetData().HashTreeRoot()
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not tree hash attestation: %v", err)


Suggested change

return nil, status.Errorf(codes.Internal, "Could not tree hash attestation: %v", err)

return nil, status.Errorf(codes.Internal, "Could not get attestation hash tree: %v", err)

saolyn · 2024-07-11T09:38:03Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_attestations.go

 		if err != nil {
-			continue
+			return nil, errors.Wrap(err, "failed to create attestation ID")


Keeping errors the same

Suggested change

return nil, errors.Wrap(err, "failed to create attestation ID")

return nil, errors.Wrap(err, "could not create attestation ID")

prestonvanloon

LGTM - Please address @saolyn comments in a follow up PR

rkapka changed the title ~~Eip 7549 vc~~ EIP-7549: validator client Jun 28, 2024

rkapka force-pushed the eip-7549-vc branch from c587632 to 9e49281 Compare July 2, 2024 13:10

rkapka added Ready For Review A pull request ready for code review API Api related tasks Validator Client Electra electra hardfork labels Jul 2, 2024

rkapka marked this pull request as ready for review July 2, 2024 13:13

rkapka requested a review from a team as a code owner July 2, 2024 13:13

rkapka requested review from terencechain, nisdas and james-prysm July 2, 2024 13:13

rkapka force-pushed the eip-7549-vc branch from 46747b3 to 9e49281 Compare July 2, 2024 13:16

prestonvanloon reviewed Jul 2, 2024

View reviewed changes

rkapka force-pushed the eip-7549-vc branch from 41e5aec to c0e3963 Compare July 4, 2024 11:27

rkapka added 6 commits July 8, 2024 19:35

EIP-7549: validator client

5a5b615

server code

9c5e6c4

tests

749d682

build fix

d1f2bd8

review

16fed79

remove context

8af1b60

rkapka force-pushed the eip-7549-vc branch from c0e3963 to 8af1b60 Compare July 8, 2024 17:36

Merge branch 'develop' into eip-7549-vc

dfff927

prestonvanloon previously approved these changes Jul 8, 2024

View reviewed changes

rkapka enabled auto-merge July 8, 2024 17:56

rkapka added 2 commits July 10, 2024 22:02

Revert "Auxiliary commit to revert individual files from 16fed79"

af85fac

This reverts commit f59e1459f3f7561e0483bc8542110794951585c5.

Merge branch '__develop' into eip-7549-vc

c14f11e

rkapka dismissed prestonvanloon’s stale review via c14f11e July 10, 2024 20:02

saolyn reviewed Jul 11, 2024

View reviewed changes

prestonvanloon approved these changes Jul 11, 2024

View reviewed changes

rkapka added this pull request to the merge queue Jul 11, 2024

Merged via the queue into develop with commit 365c625 Jul 11, 2024
16 of 17 checks passed

rkapka deleted the eip-7549-vc branch July 11, 2024 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-7549: validator client #14158

EIP-7549: validator client #14158

rkapka commented Jun 28, 2024 •

edited

Loading

prestonvanloon left a comment

prestonvanloon Jul 2, 2024

prestonvanloon Jul 2, 2024

prestonvanloon Jul 2, 2024

rkapka Jul 4, 2024

prestonvanloon Jul 8, 2024

prestonvanloon Jul 2, 2024

rkapka Jul 4, 2024

prestonvanloon Jul 2, 2024

saolyn Jul 11, 2024

saolyn Jul 11, 2024

prestonvanloon left a comment

	return nil, status.Errorf(codes.Internal, "Could not tree hash attestation: %v", err)
	return nil, status.Errorf(codes.Internal, "Could not get attestation hash tree: %v", err)

	return nil, errors.Wrap(err, "failed to create attestation ID")
	return nil, errors.Wrap(err, "could not create attestation ID")

EIP-7549: validator client #14158

EIP-7549: validator client #14158

Conversation

rkapka commented Jun 28, 2024 • edited Loading

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon left a comment

Choose a reason for hiding this comment

rkapka commented Jun 28, 2024 •

edited

Loading